-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use factory from titiler.xarray #72
base: dev
Are you sure you want to change the base?
Conversation
src/titiler/xarray_api/factory.py
Outdated
variable: Annotated[ | ||
Optional[str], | ||
Query(description="Xarray Variable"), | ||
] = None, | ||
group: Annotated[ | ||
Optional[int], | ||
Query( | ||
description="Select a specific zarr group from a zarr hierarchy, can be for pyramids or datasets. Can be used to open a dataset in HDF5 files." | ||
), | ||
] = None, | ||
decode_times: Annotated[ | ||
bool, | ||
Query( | ||
title="decode_times", | ||
description="Whether to decode times", | ||
), | ||
] = True, | ||
drop_dim: Annotated[ | ||
Optional[str], | ||
Query(description="Dimension to drop"), | ||
] = None, | ||
datetime: Annotated[ | ||
Optional[str], Query(description="Slice of time to read (if available)") | ||
] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is within the reader_dependency
https://github.com/developmentseed/titiler/blob/a1955706f02671cefac7e2806e43bab46f2a04dd/src/titiler/xarray/titiler/xarray/dependencies.py#L14-L56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but I need the url
parameter to be Optional
for this endpoint. is there a way to override the type of a particular parameter in a dependency?
when no url
is provided the user gets the interactive form instead of the actual map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to override the type of a particular parameter in a dependency?
Yes but not if it's only for a specific endpoint, you'll need to hardcode things directly at the endpoint level
I wasn't talking about the url
parameter but maybe a better way of doing this would be to have separate endpoints for both webpage!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it might make more sense to have two endpoints like /map
and /map-builder
instead of this conditional logic.
name = "titiler.xarray" | ||
description = "TiTiler extension for xarray." | ||
name = "titiler-multidim" | ||
description = "TiTiler application extension for titiler.xarray." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description = "TiTiler application extension for titiler.xarray." | |
description = "TiTiler application using titiler.xarray." |
?
from titiler.core.dependencies import ColorFormulaParams, DefaultDependency | ||
from titiler.core.resources.enums import ImageType | ||
from titiler.core.resources.responses import JSONResponse | ||
from titiler.multidim.reader import XarrayReader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking but now that I'm looking at this I am wondering if the naming convention should help distinguish local application dependencies vs imported dependencies... like titiler_app.multidim|xarray or titiler.app.multidim|xarrat
document.addEventListener('DOMContentLoaded', function() { | ||
var colormapDropdown = document.getElementById("colormap-dropdown"); | ||
|
||
// Fetch colormaps from API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏽
<label for="reference">Is this URL for a reference file (i.e. <a href="https://fsspec.github.io/kerchunk">kerchunk</a>)?:</label> | ||
<br /> | ||
<br /> | ||
<input type="checkbox" id="consolidated" name="consolidated" checked> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting that I think it's ok if we remove the consolidated parameter as by default xarray will try and read consolidated and fallback to reading unconsolidated (https://docs.xarray.dev/en/v2023.06.0/generated/xarray.open_zarr.html)
I see this was discussed here: developmentseed/titiler#1016 (comment) So no more Kerchunk support? While I see how virtual Zarr is the better approach, I think some projects that are looking to use TiTiler for multidim datasets are still using Kerchunk. For example on the UKRI / UK EO DataHub, this may be data they need to load with TiTiler: https://radiantearth.github.io/stac-browser/#/external/eocis.org/stac/collections/eocis-lst-slstrA-day Example reference file: https://eocis.org/data/eocis-lst-slstrA-day-kerchunk/2024/10/EOCIS_-LST-L3C-LST-SLSTRA-0.01deg_1DAILY_DAY-20241020000000-fv4.00-kerchunk.json Would there be an easy enough way for them to enable this in a custom runtime / fork of TiTiler-Xarray, if they need it? Not a blocker for merging this, but we should point out ways forward to them. I see VirtualiZarr and Icechunk have docs on migrating from Kerchunk / creating virtual datasets:
👌 |
Thanks for the reviews @j08lue and @abarciauskas-bgse!
It don't think it would be too hard to keep that feature working in this application! If it is important to any users right now we can define a custom version of It will be very simple if we can identify kerchunk reference files from the |
If it is simple, either via Maybe mark Kerchunk support for deprecation already, to nudge existing users time to move to Icechunk or so. I guess this adds a few dependencies, though, which would be great to be able to avoid longer-term. |
Just a reminder, we've drop kerchunk reference support in titiler-xarray because it needed some nested option (e.g you could have your kerchunk on one bucket in S3 but the netcdf on another bucket but maybe private, meaning that you need to be able to define both We could re-add it back but with the assumption that the reference file will always sit next to the data |
@j08lue I discussed briefly with @hrodmn - I agree we should add back support so we can support the UKRI / UK EO DataHub datasets. However I'm not sure it makes sense to add it to this application specifically since the deployment will only work with datasets in S3 (due to the lambda's subnet configuration), so even if we had back reference support it won't work for the example https://eocis.org/data/eocis-lst-slstrA-day-kerchunk/2024/10/EOCIS_-LST-L3C-LST-SLSTRA-0.01deg_1DAILY_DAY-20241020000000-fv4.00-kerchunk.json. When would you like to demonstrate this functionality to UKRI / UK EO DataHub? I think it would make the most sense to create a separate demo deployment for them which has this reference support. |
Thanks for being so considerate. The UK EODH engineering team is deploying their own instance of TiTiler-Xarray (in their k8s cluster), so we do not need a (demo) deployment that works for their datasets with references. It would just be great if they could keep using TiTiler-Xarray and did not need to build their own separate application for reference support. Or if, that we could describe how that is done. If I understand you correctly, TiTiler-Xarray will get back support for references, but it is a matter of the deployment to make sure the service has access to the relevant resources. |
zarr-developers/VirtualiZarr#321 Added this issue for reference with using VirtualiZarr to create Icechunk stores. There is also an issue converting an existing Kerchunk file to Icechunk using VirtualiZarr due to 'inline' data, where chunks are written as base64 chunks into the file, instead of as a reference. Conversion is not currently possible for files where this is the case - which covers a significant portion of the CEDA kerchunk files. |
Now we can import features from
titiler.xarray
and just make a few modifications instead of defining custom methods for everything 🥳titiler.xarray.io.Reader
/variables
and/histogram
endpoints totitiler.xarray.factory.TilerFactory
, and has the custom/map
endpoint template.There are a few things that will change in this application as a result, though.
dropped features
If we want to keep either of these we will need to keep the custom factory methods rather than recycle the existing factory methods from
titiler.xarray
andrio-tiler
:multiscale
zarr group zoom level functionality (Add Xarray sub-package titiler#1013 (comment))smaller changes
tileMatrixSetId
is now a required parameter for most endpoints (removed default of WebMercatorQuad)minzoom
andmaxzoom
parameters have been removed from the/info
responseband_metadata
andband_description
are no longer empty in/info
responsebounds
in thetilejson.json
response are no longer capped to-180, -90, 180, 90
because this chunk is not defined in therio-tiler
tilejson
methodtitiler-xarray/titiler/xarray/factory.py
Lines 413 to 416 in 1f31282
I will need to update the test expectations to match these feature changes but want to check with others first!
Unfortunately the renaming operation (titiler/xarray -> titiler/xarray_api makes the PR diff less useful than it could be for factory.py :/ so I will post a diff here:
factory.py diff
Resolves #71